Skip to content

Switch to the Cleaner API #1702

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Apr 1, 2025
Merged

Switch to the Cleaner API #1702

merged 1 commit into from
Apr 1, 2025

Conversation

@otbutz
Copy link
Contributor

@otbutz otbutz commented Jan 2, 2025 •
edited
Loading

Fixes #1465

  • replaced the current implementation that relies on finalize() by using a Cleaner instead
  • the ResourceTracker no longer holds a reference to the resource, as the Cleaner API mandates that:

    The cleaning action is invoked only after the associated object becomes phantom reachable, so it is important that the object implementing the cleaning action does not hold references to the object.

  • a flag is now used to detect whether the nonDisposedReporter should be invoked.
  • the tracker is registered during init() to avoid false-negatives for resources throwing an exception in their constructor. e.g new Cursor(display, -100)

@otbutz otbutz marked this pull request as ready for review January 2, 2025 13:31
@otbutz otbutz mentioned this pull request Jan 2, 2025
Closed
@github-actions
Copy link
Contributor

github-actions bot commented Jan 2, 2025 •
edited
Loading

Test Results

   545 files  + 6     545 suites  +6   33m 25s ⏱️ + 8m 19s
 4 367 tests +37   4 351 ✅ +35   16 💤 +3  0 ❌  - 1 
16 616 runs  +37  16 502 ✅ +35  114 💤 +3  0 ❌  - 1 

Results for commit a505cf1. ± Comparison against base commit 536fea8.

♻️ This comment has been updated with latest results.

@iloveeclipse
Copy link
Member

iloveeclipse commented Feb 1, 2025

@SyntevoAlex : are you interested in a review of this PR?

@SyntevoAlex
Copy link
Member

SyntevoAlex commented Feb 3, 2025

Please proceed without me.

@HannesWell
Copy link
Member

HannesWell commented Feb 3, 2025

I can have a look at this in the next days, sorry for the delayed reply.

jonahgraham
jonahgraham reviewed Feb 3, 2025
View reviewed changes
Copy link
Contributor

@jonahgraham jonahgraham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is great to see the removal of finalize. The not disposed reporting has been really useful. Having it be ready for the future when finalize is removed is the right step forward. Thanks for taking this on.

I have added some small suggestions/commentary.

laeubi
laeubi reviewed Feb 4, 2025
View reviewed changes
@otbutz
Copy link
Contributor Author

otbutz commented Feb 4, 2025

@jonahgraham I think the ResourceTrackerThreadFactory should be close enough to what the InnocuousThreadFactory does.

jonahgraham
jonahgraham approved these changes Feb 4, 2025
View reviewed changes
Copy link
Contributor

@jonahgraham jonahgraham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have tried out the patch on Linux and it looks good to me. I tested it both with the manual test org.eclipse.swt.tests.manual.Bug569752_DetectNonDisposedOsResources and by running the IDE (with a couple of dispose calls commented out to see the reporting in operation - and heap status on so I can trigger GC)

This looks good to me. As this code sits so completely core to everything SWT I would prefer if someone else approved too before it gets merged.

@jonahgraham I think the ResourceTrackerThreadFactory should be close enough to what the InnocuousThreadFactory does.

It looks like it to me.

@otbutz
Copy link
Contributor Author

otbutz commented Feb 4, 2025 •
edited
Loading

Sorry for the last minute change. I've moved the cleaner from the Resource class to the ResourceTracker class. The advantage of this is that the cleaner thread is not started until the first instance of a ResourceTracker is created, as specified in JLS §12.4.1

@jonahgraham
Copy link
Contributor

jonahgraham commented Feb 4, 2025

Sorry for the last minute change. I've moved the cleaner from the Resource class to the ResourceTracker class. The advantage of this is that the cleaner thread is not started until the first instance of a ResourceTracker is created, as specified in JLS §12.4.1

I did a recheck with the latest commit. It looks fine to me.

@otbutz
Copy link
Contributor Author

otbutz commented Feb 4, 2025 •
edited
Loading

Missed a call to Cleanable.clean() within ignoreNonDisposed().

Edit: This should have no effect, since the only caller of this method never calls init() in the first place. No idea why unit tests fail now 🤷‍♂️

@HannesWell
Copy link
Member

HannesWell commented Feb 5, 2025

This looks good to me. As this code sits so completely core to everything SWT I would prefer if someone else approved too before it gets merged.

I just had a brief look and generally this looks sane, but I can have a detailed second review tomorrow.

@otbutz is this an urgent change for you that should be available for the March release? If not we could postpone the submission to the beginning of the next release cycle to have more time to find potential, unexpected drawbacks.

In general @otbutz could you please squash all commits into one with a commit message that covers and if necessary explains the overall change?

@otbutz
Copy link
Contributor Author

otbutz commented Feb 5, 2025

Feel free to postpone. The current finalize-based implementation works fine, and it would be unwarranted to rush a replacement that hasn't been properly vetted.

@otbutz otbutz force-pushed the patch-2 branch 2 times, most recently from 72dc6f8 to ff57791 Compare February 5, 2025 07:52
@iloveeclipse
Copy link
Member

iloveeclipse commented Mar 26, 2025

I've only rebased on latest SWT state.

@iloveeclipse iloveeclipse mentioned this pull request Apr 1, 2025
Closed
iloveeclipse
iloveeclipse approved these changes Apr 1, 2025
View reviewed changes
Copy link
Member

@iloveeclipse iloveeclipse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@iloveeclipse iloveeclipse merged commit c7712e4 into eclipse-platform:master Apr 1, 2025
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@laeubi laeubi laeubi left review comments

@jonahgraham jonahgraham jonahgraham approved these changes

@iloveeclipse iloveeclipse iloveeclipse approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Avoid overriding finalize()

6 participants

@otbutz @iloveeclipse @SyntevoAlex @HannesWell @jonahgraham @laeubi